Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modularise scripts #1433

Merged
merged 8 commits into from
Mar 4, 2017
Merged

Conversation

djgrant
Copy link
Contributor

@djgrant djgrant commented Jan 23, 2017

Following the discussion and for reasons mentioned in #1431, this PR aims to refactor the webpack abstractions found in the start script out into their own utility modules.

I've aimed to maintain the general narrative of the script by keeping the running of methods in the script and using callback to eliminate interweaving.

Summary of changes

/config
  + webpackDevServer.config.js
/scripts
  /utils
     + addWebpackMiddleware.js
     ~ createJestConfig.js
     + createWebpackCompiler.js
  ~ eject.js
  ~ start.js 
  • All files found in specified folders get copied to the app directory on eject
  • Except files flagged with a // @remove-file-on-eject comment, which will be skipped i.e. scripts/eject.js & scripts/utils/createJestConfig.js

@djgrant
Copy link
Contributor Author

djgrant commented Jan 23, 2017

@gaearon Thinking that the dev server config could just be moved to config/ instead.

@gaearon
Copy link
Contributor

gaearon commented Jan 24, 2017

This currently breaks CI after ejecting: https://travis-ci.org/facebookincubator/create-react-app/jobs/194654920

I think you might need to add more files to the copying script in scripts/eject.js.

@djgrant
Copy link
Contributor Author

djgrant commented Jan 26, 2017

I've introduced a convention of adding // @remove-file-on-eject to the top of files that are not to be copied over at ejection. This removes the need for a whitelist of files and allows me to put the utils folder inside of scripts where it can be used in an ejected app.

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

This looks pretty good. I'm sorry we missed it.
Can you please rebase this with latest changes?

@gaearon gaearon added this to the 0.10.0 milestone Feb 24, 2017
@djgrant djgrant force-pushed the enhancement/modularise-scripts branch from 24f5b35 to 4908921 Compare February 26, 2017 16:54
@djgrant
Copy link
Contributor Author

djgrant commented Feb 26, 2017

@gaearon no worries. Now rebased.

@Timer
Copy link
Contributor

Timer commented Mar 3, 2017

Is this functionally equivalent? If so, this looks great -- I feel like this is something we need to have rebased and just merged immediately to prevent merge issues one after another.

My only concern is if this works with our recently-added support of linking react-scripts; but that's something I'd be willing to test after getting this in. Only reason I asked is because of the new method of getting files to eject.

@gaearon, if this is rebased again can we merge immediately? I don't foresee any more script changes we're making in the 0.9.x branch.

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2017

I'm fine with merging now but let's make sure we don't lose any recent changes accidentally.

That is, the reviewer needs to actually read through those functions and make sure the contents match.

@Timer
Copy link
Contributor

Timer commented Mar 3, 2017

I'll review this.

@Timer Timer force-pushed the enhancement/modularise-scripts branch from 4908921 to 38bc01c Compare March 3, 2017 21:42
@Timer
Copy link
Contributor

Timer commented Mar 3, 2017

Should be good, @gaearon -- I believe I captured all the missing changes in 38bc01c.

}

if (showInstructions) {
if (typeof onReadyCallback === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is confusing that we only call it when showInstructions is true.
We should instead pass it as an argument.

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2017

Looks good aside from one nit.

@Timer
Copy link
Contributor

Timer commented Mar 4, 2017

Before:

Copying files into /Users/joe/Desktop/test2
  Adding config/env.js to the project
  Adding config/paths.js to the project
  Adding config/polyfills.js to the project
  Adding config/webpack.config.dev.js to the project
  Adding config/webpack.config.prod.js to the project
  Adding config/jest/cssTransform.js to the project
  Adding config/jest/fileTransform.js to the project
  Adding scripts/build.js to the project
  Adding scripts/start.js to the project
  Adding scripts/test.js to the project

After:

Copying files into /Users/joe/Desktop/test1
  Adding /config/env.js to the project
  Adding /config/paths.js to the project
  Adding /config/polyfills.js to the project
  Adding /config/webpack.config.dev.js to the project
  Adding /config/webpack.config.prod.js to the project
  Adding /config/webpackDevServer.config.js to the project
  Adding /config/jest/babelTransform.js to the project
  Adding /config/jest/cssTransform.js to the project
  Adding /config/jest/fileTransform.js to the project
  Adding /scripts/build.js to the project
  Adding /scripts/start.js to the project
  Adding /scripts/test.js to the project
  Adding /scripts/utils/addWebpackMiddleware.js to the project
  Adding /scripts/utils/createWebpackCompiler.js to the project

Looks like we accidentally included /config/jest/babelTransform.js. Removed in 61e99ee.

@Timer
Copy link
Contributor

Timer commented Mar 4, 2017

Thanks so much for this, @djgrant! Your efforts will help many people who maintain forks of create-react-app. 😄

I hope I didn't upset you by making a few changes, we really wanted to get this in so we stopped breaking it every few days.

Merging once CI is green.

@Timer
Copy link
Contributor

Timer commented Mar 4, 2017

AppVeyor CI green on my repo.

@Timer Timer merged commit b88d665 into facebook:master Mar 4, 2017
@gaearon gaearon mentioned this pull request Mar 4, 2017
@djgrant djgrant deleted the enhancement/modularise-scripts branch March 4, 2017 19:49
@djgrant
Copy link
Contributor Author

djgrant commented Mar 4, 2017

Smashing, great to see this in master! Thanks @Timer and @gaearon for ironing it out!

path.join('scripts', 'test.js')
];
// Make shallow array of files paths
var files = folders.reduce(function (files, folder) {
Copy link
Contributor

@tuchk4 tuchk4 Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Timer btw. Is there is any code convention (besides of eslint)? Use normal function or arrow?
Right now there are both normal and arrow are using. For example - eject.js.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have a lower node requirement than Node 4, iirc. I'm not sure on an official preference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrows are fine. If it runs on Node 4 then it's good. But only in react-scripts.

Global CLI should stay parseable by Node 0.12 so that we can show a nice error message instead of crashing. (We could also split modern code into a lazy require.)

SpaceK33z pushed a commit to CodeYellowBV/create-react-cy-app that referenced this pull request Mar 7, 2017
* Refactor start script into modules

* Move dev server config into config file

* Replace eject file whitelist with a "remove-file-on-eject" flag

* Move utils into scripts folder (for inclusion in ejection)

* Add missed changes

* Pass showInstructions as an argument

* Fix eject bug

* Don't eject babelTransform

# Conflicts:
#	packages/react-cy-scripts/scripts/eject.js
#	packages/react-cy-scripts/scripts/start.js
#	packages/react-cy-scripts/utils/createJestConfig.js
#	packages/react-scripts/scripts/utils/createJestConfig.js
#	packages/react-scripts/utils/createJestConfig.js
sbuzonas pushed a commit to sbuzonas/react-scripts that referenced this pull request May 5, 2017
* Refactor start script into modules

* Move dev server config into config file

* Replace eject file whitelist with a "remove-file-on-eject" flag

* Move utils into scripts folder (for inclusion in ejection)

* Add missed changes

* Pass showInstructions as an argument

* Fix eject bug

* Don't eject babelTransform
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants